Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: app-hash mismatch if upgrade migration commit is interrupted(backport cosmos/cosmos-sdk#13530) #1310

Merged
merged 7 commits into from
Mar 27, 2024
Merged

fix: app-hash mismatch if upgrade migration commit is interrupted(backport cosmos/cosmos-sdk#13530) #1310

merged 7 commits into from
Mar 27, 2024

Conversation

jaeseung-bae
Copy link
Contributor

@jaeseung-bae jaeseung-bae commented Mar 27, 2024 •
edited
Loading

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@jaeseung-bae jaeseung-bae self-assigned this Mar 27, 2024
@CLAassistant
Copy link

CLAassistant commented Mar 27, 2024 •
edited
Loading

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ jaeseung-bae
❌ mmsqe
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov Codecov
Copy link

codecov bot commented Mar 27, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.42%. Comparing base (e5709f3) to head (ab6defa).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1310   +/-   ##
=======================================
  Coverage   70.41%   70.42%           
=======================================
  Files         643      643           
  Lines       54759    54765    +6     
=======================================
+ Hits        38560    38566    +6     
  Misses      14024    14024           
  Partials     2175     2175
Files Coverage Δ
store/rootmulti/store.go 74.59% <100.00%> (+0.27%) ⬆️

@jaeseung-bae jaeseung-bae marked this pull request as ready for review March 27, 2024 01:28
@jaeseung-bae jaeseung-bae added A: bug Something isn't working A: State Machine Breaking Any changes that result in a different AppState given same genesisState and txList. C:store labels Mar 27, 2024
0Tech
0Tech reviewed Mar 27, 2024
View reviewed changes
Copy link
Collaborator

@0Tech 0Tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a breaking change? Theoretically, yes it is, but the consequence would be chain halt. In my opinion, we may backport the fix. I'd like to get opinions from other reviewers.

@jaeseung-bae
Copy link
Contributor Author

jaeseung-bae commented Mar 27, 2024 •
edited
Loading

Is it a breaking change? Theoretically, yes it is, but the consequence would be chain halt. In my opinion, we may backport the fix. I'd like to get opinions from other reviewers.

Thanks for the comment. I was a little bit confused with whether it's breaking change or not.

@jaeseung-bae
Copy link
Contributor Author

jaeseung-bae commented Mar 27, 2024 •
edited
Loading

Is it a breaking change? Theoretically, yes it is, but the consequence would be chain halt. In my opinion, we may backport the fix. I'd like to get opinions from other reviewers.

Thanks for the comment. I was a little bit confused with whether it's breaking change or not.

After I re-check, I concluded that it's not a consensus breaking change.

For reviewers, please feel free to comment any opinions on this.

@jaeseung-bae jaeseung-bae added backport/v0.48.x and removed A: State Machine Breaking Any changes that result in a different AppState given same genesisState and txList. labels Mar 27, 2024
@jaeseung-bae jaeseung-bae requested review from 0Tech, ulbqb and 170210 March 27, 2024 02:39
@ulbqb
Copy link
Member

ulbqb commented Mar 27, 2024

I think this issue is about replaying blocks. Consensus wouldn't be related.

@jaeseung-bae jaeseung-bae merged commit 9105ff4 into Finschia:main Mar 27, 2024
37 of 38 checks passed
@jaeseung-bae jaeseung-bae deleted the fix/app-hash-mismatch branch March 27, 2024 10:55
mergify bot pushed a commit that referenced this pull request Mar 27, 2024
@jaeseung-bae @mergify
fix: app-hash mismatch if upgrade migration commit is interrupted(bac…
1139b35 
…kport cosmos/cosmos-sdk#13530) (#1310)

* fix: possible app-hash mismatch(cherry-pick cosmos-sdk #13530)

* chore: fix testcase

* chore: update changelog

* chore: lint fix

* chore: not a breaking change

---------

Co-authored-by: mmsqe <[email protected]>
(cherry picked from commit 9105ff4)

# Conflicts:
#	CHANGELOG.md
@mergify mergify bot mentioned this pull request Mar 27, 2024
Merged
5 tasks
0Tech added a commit that referenced this pull request Mar 29, 2024
@mergify @jaeseung-bae @0Tech
fix: app-hash mismatch if upgrade migration commit is interrupted(bac…
feb8821 
…kport cosmos/cosmos-sdk#13530) (backport #1310) (#1318)

* fix: app-hash mismatch if upgrade migration commit is interrupted(backport cosmos/cosmos-sdk#13530) (#1310)

* fix: possible app-hash mismatch(cherry-pick cosmos-sdk #13530)

* chore: fix testcase

* chore: update changelog

* chore: lint fix

* chore: not a breaking change

---------

Co-authored-by: mmsqe <[email protected]>
(cherry picked from commit 9105ff4)

# Conflicts:
#	CHANGELOG.md

* Update CHANGELOG.md

---------

Co-authored-by: jaeseung-bae <[email protected]>
Co-authored-by: Youngtaek Yoon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@170210 170210 170210 approved these changes

@ulbqb ulbqb ulbqb approved these changes

@tkxkd0159 tkxkd0159 Awaiting requested review from tkxkd0159 tkxkd0159 is a code owner

@zemyblue zemyblue Awaiting requested review from zemyblue zemyblue is a code owner

@0Tech 0Tech Awaiting requested review from 0Tech 0Tech is a code owner

Assignees

@jaeseung-bae jaeseung-bae

Labels
A: bug Something isn't working backport/v0.48.x C:store
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jaeseung-bae @CLAassistant @ulbqb @0Tech @170210 @mmsqe